fix(draft): credit a Swiss bye as a match win (odd pods) - #6351
Conversation
An odd-pod Swiss round leaves one player unpaired (a bye), but the bye was never credited — match_wins stayed 0, so Swiss standings (sorted by match_wins) undercounted the bye player. generate_swiss_pairings now reports the bye alongside the pairings, and apply_generate_pairings credits it a match win via the same ensure_match_record(..).match_wins += 1 path used for a normal win. SingleElimination is unaffected (it already rejects non-8 pods). Added a regression test: a 3-seat Swiss round-1 credits exactly one match win (the bye); it fails on the pre-fix code (0 != 1) and passes after.
superagent-security
Bot
added
the
contributor:flagged
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSwiss pairing generation now validates sequential rounds, returns the unpaired player as an optional bye, and credits that player with a match win. Tests verify the bye recipient and reject duplicate round generation without duplicate credit. ChangesSwiss bye match-win credit
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/draft-core/src/session.rs (1)
1749-1752: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the specific bye player’s win.
The aggregate total passes even if a paired player is credited. Derive the unpaired
PlayerIdfrom the generated pairing and assert that record has exactly onematch_win, while paired players remain unchanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/draft-core/src/session.rs` around lines 1749 - 1752, Replace the aggregate total_match_wins assertion in the bye-handling test with an assertion targeting the specific unpaired PlayerId from the generated pairing. Derive that bye player, verify its match record has exactly one match_win, and retain checks that paired players’ records remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/draft-core/src/session.rs`:
- Around line 131-143: Make the GeneratePairings flow around
generate_swiss_pairings and the new_pairings insertion idempotent for repeated
calls on the same Swiss round. Detect whether pairings for round already exist
before appending or crediting swiss_bye, and ensure an existing round cannot
duplicate pairings or increment match_wins again while preserving normal
generation for new rounds.
---
Nitpick comments:
In `@crates/draft-core/src/session.rs`:
- Around line 1749-1752: Replace the aggregate total_match_wins assertion in the
bye-handling test with an assertion targeting the specific unpaired PlayerId
from the generated pairing. Derive that bye player, verify its match record has
exactly one match_win, and retain checks that paired players’ records remain
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e09f8399-2e0d-4689-9c83-f69ed57fd3f1
📒 Files selected for processing (1)
crates/draft-core/src/session.rs
| let (new_pairings, swiss_bye) = match session.config.tournament_format { | ||
| TournamentFormat::Swiss => generate_swiss_pairings(session, round, &mut rng), | ||
| TournamentFormat::SingleElimination => generate_se_pairings(session, round), | ||
| TournamentFormat::SingleElimination => (generate_se_pairings(session, round), None), | ||
| }; | ||
|
|
||
| for p in &new_pairings { | ||
| session.pairings.push(p.clone()); | ||
| } | ||
| // A Swiss bye counts as a match win for the unpaired player; without this credit an | ||
| // odd-pod bye scores nothing and Swiss standings (sorted by match_wins) are wrong. | ||
| if let Some(bye) = swiss_bye { | ||
| ensure_match_record(&mut session.match_records, bye).match_wins += 1; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make Swiss bye credit idempotent.
GeneratePairings is permitted after RoundComplete, but this path does not verify that round is new or that pairings for it are absent. Repeating generation for an odd Swiss round can append another pairing set and increment bye standings again, corrupting match totals. Validate round uniqueness/monotonicity or make pairing insertion and bye credit idempotent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/draft-core/src/session.rs` around lines 131 - 143, Make the
GeneratePairings flow around generate_swiss_pairings and the new_pairings
insertion idempotent for repeated calls on the same Swiss round. Detect whether
pairings for round already exist before appending or crediting swiss_bye, and
ensure an existing round cannot duplicate pairings or increment match_wins again
while preserving normal generation for new rounds.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Blocked — replaying an already generated Swiss round duplicates pairings and the bye win.
🔴 Blocker
crates/draft-core/src/session.rs:91-143 permits GeneratePairings from RoundComplete and appends generated pairings plus swiss_bye without checking whether round already exists. Since round is supplied by the action, replaying an existing odd Swiss round appends a second set of pairings and increments the bye player’s match_wins again, corrupting standings. The CodeRabbit finding is confirmed against the current head.
crates/draft-core/src/session.rs:1749-1752 only asserts the aggregate win total, so it also passes if the credit goes to a paired player. Add a regression that attempts the same round again after RoundComplete, verifies no duplicate pairing/bye credit (or a clear transition error), and asserts the specific unpaired player has exactly one match win while the paired players remain unchanged.
Recommendation: request changes for round uniqueness/idempotence and a discriminating specific-bye test.
|
This pull request still has unresolved requested changes, and there has been no contributor follow-up on the current head. Please push a commit that addresses the existing requested changes and request re-review within 7 days of this warning. If the requested changes remain unaddressed after that window, this pull request will be automatically closed. |
|
Maintainer follow-up: the prior current-head review found the Swiss bye/replay fix correct, but CI failed only |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head re-review approved. The sequential-round guard rejects replay before any pairing or bye mutation, and the regression identifies the specific bye player while confirming paired players remain unchanged. All current checks are green.
2 participants
Summary
Fixes #6349: in an odd-sized Swiss pod,
generate_swiss_pairings(crates/draft-core/src/session.rs) leaves one player unpaired each round — they take a bye — but the bye was never credited.match_winsstayed 0, so Swiss standings (sorted bymatch_wins) undercounted the bye player.DraftSession::newaccepts anyseats.len()(Swiss has no even/8 guard, unlike SingleElimination), so odd pods are reachable.generate_swiss_pairingsnow returns the round's pairings plus the bye player (Option<PlayerId>), andapply_generate_pairingscredits the bye a match win via the sameensure_match_record(..).match_wins += 1path a normal win uses. SingleElimination is unaffected.Files changed
crates/draft-core/src/session.rs— return the bye fromgenerate_swiss_pairings; credit it inapply_generate_pairings; regression test.Implementation method
Method: not-applicable — this is draft-core tournament pairing/standings, not
crates/engine/game logic, so/engine-implementerdoes not apply. Targeted fix (return-type widened to report the bye) + one regression test; no new types or public API beyond the internal fn's return shape.CR references
None — Swiss tournament structure is not part of the MTG Comprehensive Rules (CR governs game rules; pairing/standings are tournament policy).
Track
Developer — Rust
nightly-2026-04-19(the repo's pinned toolchain) is installed locally. I built and ran the test on the real workspace build (not just CI):cargo test -p draft-core swiss_bye_in_odd_pod→ 1 passed.left: 0, right: 1— so the test genuinely catches the bug.clippydeferred to CI: repeated localcargo clippyruns were cut short by environment build-lock/timeout contention; the change mirrors existing idioms in the same file, so no new lint surface is expected.LLM
Model: claude-opus-4-8
Thinking: high
Verification
cargo test -p draft-core swiss_bye_in_odd_pod— ok, 1 passed (and fails0 != 1with the credit disabled — test-first verified).scripts/check-parser-combinators.sh— Gate A PASS (no parser files touched).clippy/ fullcargo test— deferred to CI (local build-lock contention).Gate A
Gate A PASS head=4378726a9aa302acf1c3f80c63c2e3ad2268210b base=21fc5f3b476021ea78e38b712cbffe0ae417a5d9
Anchored on
crates/draft-core/src/session.rs:311—apply_match_record_resultcredits a match win withensure_match_record(records, winner_pid).match_wins += 1; the bye credit reuses that exact house pattern.crates/draft-core/src/session.rs:118—apply_generate_pairingsalready special-cases tournament pod size (SingleElimination's non-8 guard); crediting the odd-Swiss bye is the same class of pod-size edge handling in the same function.Summary by CodeRabbit